[Payment due @shubham1206agra] Render thinking + draft-streaming UI for custom agent chats#90751
Conversation
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| const actionsValue = {kickoffWaitingIndicator}; | ||
| function AgentZeroStatusGate({reportID, personaAccountID, children}: React.PropsWithChildren<{reportID: string; personaAccountID: number}>) { | ||
| const {kickoffWaitingIndicator, ...indicatorState} = useAgentZeroStatusIndicator(reportID, personaAccountID); | ||
| const stateValue = useMemo(() => ({...indicatorState, personaAccountID}), [indicatorState, personaAccountID]); |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The manual useMemo for stateValue is redundant — the compiler automatically memoizes derived values. Additionally, indicatorState is a new object reference every render (from the rest-spread of the hook return), so this memo never actually caches.
Remove the wrapper and compute the value directly:
const stateValue = {...indicatorState, personaAccountID};Reviewed at: 06e69be | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in c978370 — dropped the useMemo wrapper. React Compiler handles it.
| function AgentZeroStatusGate({reportID, personaAccountID, children}: React.PropsWithChildren<{reportID: string; personaAccountID: number}>) { | ||
| const {kickoffWaitingIndicator, ...indicatorState} = useAgentZeroStatusIndicator(reportID, personaAccountID); | ||
| const stateValue = useMemo(() => ({...indicatorState, personaAccountID}), [indicatorState, personaAccountID]); | ||
| const actionsValue = useMemo(() => ({kickoffWaitingIndicator}), [kickoffWaitingIndicator]); |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The manual useMemo for actionsValue is redundant — the compiler handles this automatically.
Remove the wrapper:
const actionsValue = {kickoffWaitingIndicator};Reviewed at: 06e69be | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in c978370 — dropped the useMemo wrapper. React Compiler handles it.
| const isAgentZeroChat = isConciergeChat || isAdmin; | ||
| // First participant accountID with a SHARED_NVP_AGENT_PROMPT entry — both gates and identifies | ||
| // the persona in one pass so downstream callers can attribute streaming events. | ||
| const agentParticipantAccountID = useMemo(() => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The manual useMemo for agentParticipantAccountID is redundant — the compiler automatically caches derived computations.
Remove the wrapper:
const agentParticipantAccountID = (!participantAccountIDs?.length || !agentPrompts)
? undefined
: participantAccountIDs.find((accountID) => !!agentPrompts[`${ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT}${accountID}`]);Reviewed at: 06e69be | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in c978370 — dropped the useMemo wrapper. React Compiler handles it.
| function AgentZeroStatusProvider({reportID, children}: React.PropsWithChildren<{reportID: string | undefined}>) { | ||
| const [chatType] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {selector: getReportChatType}); | ||
| const [participantAccountIDs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {selector: getReportParticipantAccountIDs}); | ||
| const [agentPrompts] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT) subscribes to the entire agent-prompt collection. Any change to any agent prompt will re-render this provider and re-evaluate all downstream logic, even though it only needs to know whether a specific participant has an entry.
Consider narrowing the subscription — e.g., a selector that reduces the collection to just a Set of accountIDs (small primitive set), or restructure so you subscribe to individual SHARED_NVP_AGENT_PROMPT_{accountID} keys for each participant.
Reviewed at: 06e69be | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in c978370. Added a getAgentAccountIDFlags selector that reduces the full collection to a small Record<accountID, true> — only re-renders when an agent is added/removed (not on prompt-content edits). Applied to both AgentZeroStatusContext and ConciergeDraftContext.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06e69bed31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isCustomAgentChat = agentParticipantAccountID !== undefined; | ||
| const isAgentZeroChat = isConciergeChat || isAdmin || isCustomAgentChat; |
There was a problem hiding this comment.
This change enables AgentZeroStatusProvider for custom-agent DMs, but the sibling ConciergeDraftProvider in ReportScreen still only mounts for Concierge/admin chats (ConciergeDraftContext.tsx keeps isAgentZeroChat = isConciergeChat || isAdmin). In a custom-agent DM, that means status/thinking can render while draft subscriptions never start, so CONCIERGE_DRAFT_* events are dropped and the streamed draft bubble does not appear. Please keep the gating logic aligned so custom-agent chats mount both providers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
(Yuwen's Agent) Already addressed in commit e177895 (feat(chatbot): open ConciergeDraftProvider gate for custom agent chats) — ConciergeDraftContext.tsx now uses the same detection logic as AgentZeroStatusContext (SHARED_NVP_AGENT_PROMPT entry OR agent_<id>@expensify.ai email pattern). The follow-up commit c978370 also keeps both providers' subscription/selector logic aligned.
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Core design doc issue
| import type {ConciergeDraft} from './conciergeDraftState'; | ||
| import {applyConciergeDraftEvent, getCachedDraft, setCachedDraft} from './conciergeDraftState'; | ||
|
|
||
| const CUSTOM_AGENT_EMAIL_REGEX = /^agent_(\d+)@expensify\.ai$/; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
CUSTOM_AGENT_EMAIL_REGEX, getReportParticipantAccountIDs, and getAgentAccountIDFlags are duplicated verbatim from AgentZeroStatusContext.tsx. The agent-detection logic in the provider body (lines 74-80) is also a near-duplicate of the same logic in AgentZeroStatusContext.tsx (lines 94-100).
Extract the shared constants, selectors, and detection logic into a shared utility (e.g., src/pages/inbox/agentChatUtils.ts) or a shared hook (e.g., useIsAgentZeroChat(reportID)) that both providers can import.
Reviewed at: c978370 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in 0f7f2b2. Extracted CUSTOM_AGENT_EMAIL_REGEX, getReportParticipantAccountIDs, getAgentAccountIDFlags, and a new getAgentLoginAccountIDFlags into src/selectors/AgentZeroChat.ts — both contexts import from there now.
| const [chatType] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {selector: getReportChatType}); | ||
| const [participantAccountIDs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {selector: getReportParticipantAccountIDs}); | ||
| const [agentAccountIDFlags] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT, {selector: getAgentAccountIDFlags}); | ||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Subscribing to ONYXKEYS.PERSONAL_DETAILS_LIST without a selector causes AgentZeroStatusProvider to re-render whenever any user's personal details change (avatar, display name, status, etc.), even though only the login field of a small number of participant accountIDs is needed. Consider narrowing the subscription with a selector or extracting just the needed logins, and add a justifying comment if the broad subscription is intentional.
Reviewed at: c978370 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in 0f7f2b2. Replaced the raw useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST) with useOnyx(..., {selector: getAgentLoginAccountIDFlags}) — reduces the list to a small Record<accountID, true> of accounts whose login matches agent_<id>@expensify.ai. Provider only re-renders when an agent account is added/removed (or its login changes), not on any unrelated user-detail change.
| const [chatType] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {selector: getReportChatType}); | ||
| const [participantAccountIDs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {selector: getReportParticipantAccountIDs}); | ||
| const [agentAccountIDFlags] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_AGENT_PROMPT, {selector: getAgentAccountIDFlags}); | ||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
Same issue as in AgentZeroStatusContext.tsx: subscribing to ONYXKEYS.PERSONAL_DETAILS_LIST without a selector causes re-renders on any personal details change across the entire app, when only the login field for a few participant accountIDs is needed.
Reviewed at: c978370 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
(Yuwen's Agent) Fixed in 0f7f2b2 (same selector + shared module as AgentZeroStatusContext).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@yuwenmemon Could you merge main in and resolve conflicts. Also, worth generating an Adhoc build if you want C+ review on it as well. |
…gent-client # Conflicts: # src/pages/inbox/conciergeDraftState.ts
This comment has been minimized.
This comment has been minimized.
|
@shubham1206agra bump for a review please |
| function buildConciergeDraftReportAction({bodyMarkdown, created, finalRenderedHTML, reportActionID, reportID}: BuildConciergeDraftReportActionParams): ReportAction | null { | ||
| function buildConciergeDraftReportAction({ | ||
| actorAccountID, | ||
| actorDisplayName, |
There was a problem hiding this comment.
@yuwenmemon No one passes actorDisplayName param. Is this intentional?
| * Canonical email pattern for custom-agent accounts. Mirrors the value PHP mints in | ||
| * `AgentAPI::createAgent` (`agent_<accountID>@expensify.ai`). | ||
| */ | ||
| const CUSTOM_AGENT_EMAIL_REGEX = /^agent_(\d+)@expensify\.ai$/; |
| /** | ||
| * Persona accountID this reasoning bubble should be attributed to (Concierge for Concierge | ||
| * runs, the custom agent's accountID for agent runs). Optional for backward compatibility — | ||
| * absent payloads keep the legacy Concierge attribution. | ||
| */ | ||
| actorAccountID?: number; |
There was a problem hiding this comment.
@yuwenmemon Is there any use for this field in this PR? Cause I can't see any usage.
|
Updated! |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-05-22.at.11.27.22.AM.mov |
|
🎯 @shubham1206agra, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
| @@ -41,10 +42,16 @@ const ConciergeDraftActionsContext = createContext<ConciergeDraftActions>(defaul | |||
|
|
|||
| function ConciergeDraftProvider({reportID, children}: React.PropsWithChildren<{reportID: string | undefined}>) { | |||
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The custom-agent detection logic (subscribing to participantAccountIDs via getReportParticipantAccountIDs, subscribing to agentAccountIDFlags via getAgentAccountIDFlags, and computing isCustomAgentChat) is duplicated almost verbatim between ConciergeDraftContext.tsx and AgentZeroStatusContext.tsx. Both providers perform the same three useOnyx subscriptions and the same isAgentZeroChat computation.
Extract a shared hook (e.g., useIsAgentZeroChat(reportID)) that encapsulates the agent-detection logic and returns the relevant values (isAgentZeroChat, agentParticipantAccountID, etc.). Both providers can then call that single hook instead of duplicating the subscriptions and logic.
Reviewed at: b423f9a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // First participant whose accountID has a SHARED_NVP_AGENT_PROMPT entry. Both gates and | ||
| // identifies the persona in one pass. | ||
| const agentParticipantAccountID = participantAccountIDs?.find((accountID) => !!agentAccountIDFlags?.[accountID]); | ||
| const isCustomAgentChat = agentParticipantAccountID !== undefined; | ||
| const isAgentZeroChat = isConciergeChat || isAdmin || isCustomAgentChat; |
There was a problem hiding this comment.
Custom-agent detection is gated entirely on SHARED_NVP_AGENT_PROMPT already being hydrated. So If a user opens a custom-agent chat directly after sign-in or from a link without first visiting Settings > Agents, this collection will be empty.
We need to hydrate this data before the gate runs, or identify custom-agent chats from report metadata or participants that are available on report open.
There was a problem hiding this comment.
Ah good call. Fixed! Added an eager openAgentsPage() call to AuthScreensInitHandler.tsx right after the existing App.openApp() / App.reconnectApp() bootstrap. Does that work?
| const isCustomAgentChat = participantAccountIDs?.some((accountID) => !!agentAccountIDFlags?.[accountID]); | ||
| const isAgentZeroChat = isConciergeChat || isAdmin || isCustomAgentChat; |
There was a problem hiding this comment.
|
@inimaga Updated! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.3.81-0 🚀
|
|
I reviewed the changes in this PR and the existing help site articles under No help site changes are required. This PR extends the thinking-bubble and draft-streaming UI to custom-agent chats — a feature gated behind |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.81-2 🚀
|
@inimaga please review
Explanation of Change
The server side now streams reasoning summaries, status indicators, and live draft body deltas for custom-agent chats (see Web-Expensify#52866 and Auth#21626). The client just isn't subscribing yet — the
AgentZeroStatusProvidergate at `AgentZeroStatusContext.tsx` only opens for Concierge DMs and `#admins` rooms.This PR opens the gate for custom-agent chats and threads the persona's accountID through the existing UI so the thinking bubble + streamed draft render under the agent's avatar and display name instead of Concierge's.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/636271
PROPOSAL: N/A (internal feature work; see tracking issue)
Tests
Requires:
QA Steps
Same as Tests. The custom-agent feature is gated behind `BETAS.CUSTOM_AGENT`; QA testers need that beta on their account to see the Agents settings menu and create an agent.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Kapture.2026-05-18.at.15.43.20.mp4